fix(docker): remap container UID when remote user comes from image metadata#618
Conversation
…tadata The container UID/GID remap gated on the raw devcontainer.json's remoteUser/containerUser, but those commonly come from image metadata (e.g. mcr.microsoft.com/devcontainers/* images set remoteUser=vscode). With the raw config empty, shouldUpdateUserUID returned false, the remap was skipped, and the in-container chown left the bind-mounted workspace owned by the container UID (1000). On the next up the host agent (a different UID) could no longer read the tree, reported "Couldn't find a devcontainer.json", and failed writing a default one with permission denied. Pass the merged config's resolved user identity to the remap on both the single-container and compose paths so it matches the host user.
✅ Deploy Preview for images-devsy-sh canceled.
|
✅ Deploy Preview for devsydev canceled.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/tests/up/git_repositories.go (1)
60-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
TimeoutLong()for this two-up-plus-SSH flow.This spec runs
DevsyUptwice plus an SSH round trip, which is more work than the single-up"commit hash" test that also usesTimeoutShort()(Line 42). The "pull request" checkout test, which only does a singleup, already usesTimeoutLong()(Line 57). Given image pulls and container rebuilds involved invscode-remote-try-go,TimeoutShort()here risks flakiness in CI.⏱️ Proposed timeout adjustment
}, - ginkgo.SpecTimeout(framework.TimeoutShort()), + ginkgo.SpecTimeout(framework.TimeoutLong()), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/up/git_repositories.go` around lines 60 - 85, The ginkgo spec in git_repositories.go for "should re-up a repo whose remoteUser comes from image metadata" uses TimeoutShort() even though it performs two DevsyUp calls plus an SSH check, so it should be aligned with the heavier "pull request" flow by switching this test case to TimeoutLong() in the ginkgo.SpecTimeout call. Locate the spec by its description string and update the timeout near the end of that ginkgo.It block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e/tests/up/git_repositories.go`:
- Around line 60-85: The ginkgo spec in git_repositories.go for "should re-up a
repo whose remoteUser comes from image metadata" uses TimeoutShort() even though
it performs two DevsyUp calls plus an SSH check, so it should be aligned with
the heavier "pull request" flow by switching this test case to TimeoutLong() in
the ginkgo.SpecTimeout call. Locate the spec by its description string and
update the timeout near the end of that ginkgo.It block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 268bdbc8-8cb3-4965-8920-77833c678e5b
📒 Files selected for processing (4)
e2e/tests/up/git_repositories.gopkg/devcontainer/compose.gopkg/devcontainer/single.gopkg/devcontainer/single_test.go
b12de86 to
c5241ca
Compare
1 participant
Summary
Fixes a
permission deniedfailure on re-upfor git-clone workspaces whose remote user comes from image metadata (e.g.mcr.microsoft.com/devcontainers/*images setremoteUser: vscode).The container UID/GID remap (
UpdateContainerUserUID) gated on the rawdevcontainer.json'sremoteUser/containerUser. Those fields are commonly empty because the user is defined in image metadata, not the JSON. With the raw config empty,shouldUpdateUserUIDreturned false, the remap was skipped, and the in-container workspace chown left the bind-mounted host tree owned by the container UID (1000). On the nextup, the host agent (a different UID) could no longer read the tree, reported "Couldn't find a devcontainer.json", and failed writing a default one withpermission denied.The fix passes the merged config's resolved user identity (
remoteUser/containerUser/updateRemoteUserUID) to the remap on both the single-container and compose paths, so the container user is remapped to match the host user.Changes
withResolvedUserhelper overlays the merged user identity onto the config handed to the UID remap.runContainer(single-container) andfinalizeComposeContainer(compose) now use it. Compose builds the merged config before the remap so the resolved user is available.Verification
Reproduced and fixed end-to-end on a remote
ws3-sshprovider withgithub.com/microsoft/vscode-remote-try-python:permission deniedwriting.devcontainer.json. Confirmed identical on the pre-fix commit, so this is a pre-existing bug, not a regression.vscoderemapped to the host UID (502), host tree owned by the host user, and runs 2 and 3 succeed with a clean git tree and no spurious default.devcontainer.json.Summary by CodeRabbit